Mark webhook-channel configs as encrypted by TomasLongo · Pull Request #950 · opensearch-project/common-utils

TomasLongo · 2026-04-29T10:14:23Z

Mark webhook-channel configs as encrypted

This PR supports the effort to encrypt notification channel configurations in the OpenSearch Notification-Plugin.

It checks if the url for a webhook channel is encrypted and skips url-validation if this is the case.

Caveats

This introduces external knowledge into the utility project (How encrypted urls are prefixed). An alternative would be, creating dedicated types for encrypted webhook configs inside the notification project.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tomas Longo <tlongo@sternad.de>

github-actions · 2026-04-29T10:15:43Z

PR Reviewer Guide 🔍

(Review updated until commit `c764168`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns Sensitive information exposure: The `"enc:"` prefix convention used to bypass URL validation could potentially be abused. Any caller can pass a URL starting with `"enc:"` to skip validation without the URL actually being encrypted. This could allow storing malformed or malicious URLs in the system under the guise of being encrypted, potentially leading to SSRF or other injection issues when the URL is later used.
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Magic String The prefix `"enc:"` is hardcoded as a magic string in all four model classes (Chime, Slack, MicrosoftTeams, Webhook). This should be extracted into a shared constant to avoid duplication and reduce the risk of typos or inconsistency across classes. if (!url.startsWith("enc:")) { validateUrl(url) } Bypass Risk The encrypted URL check only verifies that the URL starts with `"enc:"`. There is no validation that the remainder of the string is actually encrypted content. A user could pass `"enc:"` followed by an empty string or arbitrary plaintext, bypassing URL validation entirely without any real encryption. Consider adding a minimum length check or format validation for the encrypted payload. if (!url.startsWith("enc:")) { validateUrl(url) } Inconsistent Approach The PR description mentions a flag to mark a webhook as encrypted, but the implementation uses a URL prefix convention (`"enc:"`) instead of an explicit boolean flag. This implicit approach couples the encryption marker to the URL field format, which may cause issues if the encryption scheme changes in the future or if the prefix needs to be updated. if (!url.startsWith("enc:")) { validateUrl(url) }

github-actions · 2026-04-29T10:16:04Z

PR Code Suggestions ✨

Latest suggestions up to c764168
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Extract magic encryption prefix to shared constant The `"enc:"` prefix check is a hardcoded magic string repeated across four files. If the prefix ever changes, all four model classes must be updated consistently. Consider extracting this constant to a shared location (e.g., a companion object or a constants file) and referencing it everywhere to avoid inconsistency bugs. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-31] -if (!url.startsWith("enc:")) { +if (!url.startsWith(ENCRYPTED_PREFIX)) { validateUrl(url) } +// In a shared constants file or companion object: +// const val ENCRYPTED_PREFIX = "enc:" Suggestion importance[1-10]: 5 __ Why: Extracting the `"enc:"` magic string to a shared constant is a valid maintainability improvement, as it's repeated across four files. However, this is a minor style/maintainability concern rather than a critical bug.	Low
General	Validate encrypted URL value is non-empty Bypassing URL validation for any string that starts with `"enc:"` could allow malformed or malicious input to be stored without any validation. At minimum, you should verify that the encrypted value is non-trivially long or matches an expected format (e.g., a minimum length or a regex pattern) to prevent trivially invalid encrypted values from being accepted. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-31] if (!url.startsWith("enc:")) { validateUrl(url) +} else { + require(url.length > "enc:".length) { "Encrypted URL value cannot be empty" } } Suggestion importance[1-10]: 4 __ Why: The suggestion to add a minimum length check for encrypted URLs is a reasonable defensive measure, but the existing `require(!Strings.isNullOrEmpty(url))` check already prevents empty strings. The `"enc:"` prefix alone (4 chars) would still pass the null/empty check, so the additional validation has some merit but is a minor edge case.	Low

Previous suggestions

Suggestions up to commit 4fe3b64

Category	Suggestion	Impact
Possible issue	Ensure serialization handles new encrypted field The `isEncrypted` field is added at the end of the constructor, but since `headerParams` and `method` already have defaults, callers using named parameters are fine. However, serialization/deserialization (e.g., from JSON/XContent) may break if `isEncrypted` is not handled in the corresponding `read`/`write` methods. Ensure the XContent reader and writer for `Webhook` are updated to include `isEncrypted`, otherwise deserialized objects will always have `isEncrypted = false` regardless of stored data. src/main/kotlin/org/opensearch/commons/notifications/model/Webhook.kt [28-31] +val url: String, +val headerParams: Map<String, String> = mapOf(), +val method: HttpMethodType = HttpMethodType.POST, +val isEncrypted: Boolean = false - Suggestion importance[1-10]: 6 __ Why: The `existing_code` and `improved_code` are identical, but the suggestion raises a valid concern about XContent serialization not being updated for the new `isEncrypted` field, which could cause silent data loss during deserialization. This is a legitimate issue worth investigating, though it's a verification request rather than a concrete fix.	Low
General	Verify null/empty check applies to encrypted URLs When `isEncrypted` is true, the URL may be an encrypted/obfuscated string, so the non-empty check on `url` is still valid. However, you should also consider whether an encrypted URL could legitimately be an empty string or null — if so, the `require` check should also be conditional. More critically, if `isEncrypted` is true and the URL is empty, the error message "URL is null or empty" may be misleading. Consider keeping the null/empty check unconditional but adjusting the logic if encrypted URLs have different constraints. src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt [29-32] +require(!Strings.isNullOrEmpty(url)) { "URL is null or empty" } +if (!isEncrypted) { + validateUrl(url) +} - Suggestion importance[1-10]: 2 __ Why: The `existing_code` and `improved_code` are identical, meaning no actual code change is proposed. The suggestion only asks to verify behavior, and the current logic (always checking non-empty, only validating URL format when not encrypted) is reasonable and correct.	Low

Check for prefix instead. Signed-off-by: Tomas Longo <tlongo@sternad.de>

github-actions · 2026-04-29T12:22:29Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit fd075d2.

Path	Line	Severity	Description
src/main/kotlin/org/opensearch/commons/notifications/model/Chime.kt	30	high	URL validation is bypassed for any URL prefixed with 'enc:'. This pattern is applied consistently across Chime, Slack, MicrosoftTeams, and Webhook models. While the stated intent is to support encrypted/stored URLs, the bypass is purely prefix-based with no cryptographic verification — any user-supplied string starting with 'enc:' skips validation entirely. If the consuming code passes these values to HTTP clients without further validation or decryption, this could allow SSRF attacks or requests to arbitrary internal endpoints. Maintainers should verify that a corresponding decryption layer enforces URL validity before use.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-04-29T12:23:22Z

Persistent review updated to latest commit c764168

TomasLongo · 2026-04-29T12:42:59Z

Addressing the security issue raised by the AI-Review:

If this change is read in isolation, it is right. However, the change in the notifications-plugin uses the AES-256-GCM algorithm which checks if the encrypted ur was manipulated. Decryption would fail if the content after enc: would not actually be some encrypted data.

Since the current change is mainly driven by creating backwarcompatibility with not encrypted webhook urls, the alternative approach mentioned in the PR description might be worth considering to create a tailored mechanic for encrypted webhook-urls.

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Mark channel configs as encrypted

4fe3b64

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Don't use a flag to check if url is encrypted.

c764168

Check for prefix instead. Signed-off-by: Tomas Longo <tlongo@sternad.de>

Use constant

fd075d2

Signed-off-by: Tomas Longo <tlongo@sternad.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark webhook-channel configs as encrypted#950

Mark webhook-channel configs as encrypted#950
TomasLongo wants to merge 3 commits intoopensearch-project:mainfrom
sternadsoftware:channel-config-encryption

TomasLongo commented Apr 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

TomasLongo commented Apr 29, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

TomasLongo commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!